Add new baseline-diff command#1980
Conversation
04a5e7a to
9d4ae6d
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a new experimental x-baseline-diff command to help users understand which manifest dependencies would change when moving between two baselines (commit refs), integrating it into the vcpkg command dispatcher and localization system.
Changes:
- Adds
x-baseline-diffcommand implementation and registers it in command dispatch. - Extends configuration with a helper intended to detect whether the default registry is the built-in registry.
- Adds new localized strings for command synopsis/output headings and “no change” output.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
src/vcpkg/configuration.cpp |
Adds a helper intended to detect built-in default registry kind (currently incorrect). |
include/vcpkg/configuration.h |
Exposes the new configuration helper in the public header. |
src/vcpkg/commands.cpp |
Wires the new command into the command table (currently has a syntax error in the include). |
src/vcpkg/commands.baseline-diff.cpp |
Implements the baseline comparison by generating two install plans and diffing versions. |
include/vcpkg/commands.baseline-diff.h |
Declares the new command entrypoint/metadata. |
locales/messages.json |
Adds user-facing strings for the command and output labels (some grammar issues). |
include/vcpkg/base/message-data.inc.h |
Declares new messages for localization enforcement (some grammar issues). |
9d4ae6d to
a9849db
Compare
If we use x-update-baseline we wanted to know which packages gets updated for our manifest file.
a9849db to
4a51845
Compare
| return false; | ||
| } | ||
|
|
||
| void check_for_valid_sha(StringView sha) |
There was a problem hiding this comment.
Can this use check_commit_exists (from git.h) instead?
There was a problem hiding this comment.
I see we can't do that in the general case because this kinda just blindly overwrites the baseline with a SHA and we don't necessarily have a git repo present for that.
However that makes this kinda broken because filesystem registries' baselines are not SHAs. We should only be checking that the input is a SHA when we know that registry has that constraint.
BillyONeal
left a comment
There was a problem hiding this comment.
I think the general idea of such a command makes sense.
Most of what is here is nitpicks but the one about 'why manifest only' and 'what about non-git registries' are kinda foundational.
Should we have output like this printed by x-update-baseline instead of/in addition to what you've done here?
This needs at least one end to end test.
Thanks for the new command submission!
| check_for_valid_sha(options.command_arguments.at(0)); | ||
| check_for_valid_sha(options.command_arguments.at(1)); |
There was a problem hiding this comment.
| check_for_valid_sha(options.command_arguments.at(0)); | |
| check_for_valid_sha(options.command_arguments.at(1)); | |
| check_for_valid_sha(options.command_arguments[0]); | |
| check_for_valid_sha(options.command_arguments[1]); |
| { | ||
| if (auto default_reg = configuration.config.default_reg.get()) | ||
| { | ||
| default_reg->baseline = options.command_arguments.at(i); |
There was a problem hiding this comment.
| default_reg->baseline = options.command_arguments.at(i); | |
| default_reg->baseline = options.command_arguments[i]; |
| auto dependencies = get_manifest_dependencies(*manifest_scf, features); | ||
|
|
||
| ActionPlan plan[2]; | ||
| for (int i = 0; i < 2; ++i) |
There was a problem hiding this comment.
| for (int i = 0; i < 2; ++i) | |
| for (size_t i = 0; i < 2; ++i) |
to avoid any sugned/unsigned mismatch warnings from vector.
| msgCmdBaselineDiffSynopsis, | ||
| {"vcpkg x-baseline-diff <old_commit_sha> <newer_commit_sha>", | ||
| "vcpkg x-baseline-diff $(git rev-parse 2026.02.27) $(git rev-parse 2026.03.18)"}, | ||
| Undocumented, |
There was a problem hiding this comment.
I think we need a docs page for this.
| { | ||
| RegistryConfig synthesized_registry; | ||
| synthesized_registry.kind = JsonIdBuiltin.to_string(); | ||
| synthesized_registry.baseline = options.command_arguments.at(i); |
There was a problem hiding this comment.
| synthesized_registry.baseline = options.command_arguments.at(i); | |
| synthesized_registry.baseline = options.command_arguments[i]; |
| Triplet host_triplet) | ||
| { | ||
| const auto* manifest = paths.get_manifest(); | ||
| if (manifest == nullptr) |
There was a problem hiding this comment.
I'm not sure I agree with this design: Because the user is explicitly passing in multiple baseline SHAs, it seems to really be more of a classic mode command as other registries aren't accounted for.
It seems like what you actually wanted is effectively this same diff but before/after what x-update-baseline does?
If this stays manifest only, it seems like one of the SHAs should be the one already in the manifest?
There was a problem hiding this comment.
It seems like what you actually wanted is effectively this same diff but before/after what x-update-baseline does?
Yes. I wanted to know what x-update-baseline does mean to me in terms of port versions, or what it would mean.
There was a problem hiding this comment.
If this stays manifest only, it seems like one of the SHAs should be the one already in the manifest?
At least if there is no second one. You are right, this would be more developer friendly.
| return false; | ||
| } | ||
|
|
||
| void check_for_valid_sha(StringView sha) |
There was a problem hiding this comment.
I see we can't do that in the general case because this kinda just blindly overwrites the baseline with a SHA and we don't necessarily have a git repo present for that.
However that makes this kinda broken because filesystem registries' baselines are not SHAs. We should only be checking that the input is a SHA when we know that registry has that constraint.
BillyONeal
left a comment
There was a problem hiding this comment.
(paraphrased by @BillyONeal )
@vicroms: Anything that makes baselines more helpful and less obscure is a good thing
@AugP: This concept sounds like a good idea and I have no objections to it
Apply the print_lines nitpicks.
If we use x-update-baseline we wanted to know which packages gets updated for our manifest file.